LIVY-253. Simplify interactive session state management #236
Conversation
Current coverage is 70.97% (diff: 61.11%)@@ master #236 diff @@
==========================================
Files 92 92
Lines 4747 4747
Methods 0 0
Messages 0 0
Branches 822 823 +1
==========================================
+ Hits 3367 3369 +2
+ Misses 903 900 -3
- Partials 477 478 +1
|
| @@ -387,14 +390,14 @@ class InteractiveSession( | |||
| override def onJobFailed(job: JobHandle[Void], cause: Throwable): Unit = errorOut() | |||
|
|
|||
| override def onJobSucceeded(job: JobHandle[Void], result: Void): Unit = { | |||
| transition(SessionState.Running()) | |||
| transition(SessionState.Idle()) | |||
There was a problem hiding this comment.
The repl might be in busy state if it's a recovered session. We shouldn't assume it's idle.
There was a problem hiding this comment.
Originally it is Idle state, your modification of LIVY-244 changed it to Running state. I didn't change the semantics for session recovery unless originally it is wrong.
There was a problem hiding this comment.
LIVY-244 changes the server state to Running state so server will query repl for the actual state. The actual state could be idle or busy. Your code does change the semantics.
| serverSideState match { | ||
| case SessionState.Running() => | ||
| _state match { | ||
| case SessionState.Running() | SessionState.Idle() | SessionState.Busy() => Unit |
There was a problem hiding this comment.
With your change, running isn't a legal repl state. We can remove it.
There was a problem hiding this comment.
Yes, actually in interactive session running is not used, only idle and busy and honored.
| @@ -376,6 +376,9 @@ class InteractiveSession( | |||
| } | |||
| uriFuture onFailure { case e => warn("Fail to get rsc uri", e) } | |||
|
|
|||
| // Register this class to RSCClient as a session state listener | |||
| client.get.registerStateListener(this) | |||
There was a problem hiding this comment.
livy-server won't get notifications from repl state changes happened before registerStateListener()'s called. livy-server will not show the current repl state until repl changes its state once after connection.
There was a problem hiding this comment.
I think normally this register should be invoked earlier than RSCClient fully connected to remote driver. This is a very fast non-block operation compared to network connection operations in RSCClient.
There was a problem hiding this comment.
But it's a race condition that could happen. I wouldn't introduce a race condition to simplify something.
Originally I took a similar approach too and moved away from it after spotting this race condition.
Change-Id: I4f49354787c3dd848d4daacd04a049b27c858a82
Change-Id: I24256600ba0c2c4367ea92f8e9f8952251a93dda
|
Let's find a way to simply this without introducing race condition :). |
|
OK, let me find out another way to address it. |
With LIVY-244, InteractiveSession bring in two session state, one is from yarn and another is from repl session, this bring the complexity of session state management compared to previous code. So here propose to simplify the state management code and unify these two states.